Add a single enumerated error code set with a shared apiError helper,attach code to every { error } response, document the codes in OpenAPI, regenerate the spec, and add a guard test that all emitted codes stay in the set. - #437

The goal is right — "clients match on sentences" is a real problem and an enumerated code set with one apiError helper is the right answer. But this cannot go in as it stands, for three separate reasons.

1. It bundles two unrelated features

.changeset/api-cursor-pagination.md    22 +++
src/pagination.ts                     116 +++
src/__tests__/pagination.test.ts      268 +++

Cursor pagination is not an error-code set. It is a change to the shape of every collection response, it needs its own openapi work, and it deserves its own review — bundled here it will be approved or rejected on the strength of the other half. Please split it out; I will review it on its own and it will land faster.

That leaves the error-code change touching ~25 files, which is a reasonable size for what it does.

2. It forks isAdminAuthorized into a second copy (blocking)

src/api/usage.ts now carries its own admin check:

function isAdminAuthorized(req: FastifyRequest): boolean {
  const adminToken = process.env.ADMIN_TOKEN
  if (!adminToken) return false
  …
  return a.length === b.length && require('crypto').timingSafeEqual(a, b)
}

while the diff removes 64 lines from src/api/auth.ts. The doc comment on the original says why there should only ever be one:

This is the single implementation used by every admin route (keys and usage), so the two surfaces cannot drift apart.

They have now drifted, immediately and in a way that matters. Two PRs merged today changed what that check has to do:

• #566 replaced it with three-valued checkAdminAuth, so a deployment with no ADMIN_TOKEN answers 503 admin_not_configured rather than telling an operator their correct token is invalid. Your copy returns false, so /admin/usage would answer 401 while /admin/keys answers 503 — the exact confusion that issue was about.
• #574 added a structural onRequest hook over everything under /admin/, so the per-handler check is now belt-and-braces rather than the gate. A second, weaker implementation sitting behind the hook is dead code that looks load-bearing.

Two smaller problems in the copy itself: FastifyRequest is used without being imported, and require('crypto') is a CommonJS call in a module the rest of the file treats as ESM. Both suggest it was moved rather than written.

Whatever else changes, isAdminAuthorized / checkAdminAuth must stay in src/api/auth.ts with one caller-visible behaviour.

3. The merge against main silently produces code that does not compile

git merge origin/main reports one conflict (src/app.ts) and then hands back a broken tree. Git merges line regions, not declarations, so where both sides edited the same function it kept both. In src/api/usage.ts:

  app.get(
    '/usage/me',
    async (req, reply) => {
      …
      return reply.send(summary)
    }
    // The key presented determines the answer — never a parameter.
    const summary = await getUsageSummary(apiKey.id)
    return reply.send(summary)
  }

  app.get('/usage/me', handleCallerUsage)
  app.get('/usage', handleCallerUsage)

Your inline handler and main's extracted handleCallerUsage are both present, so the function is never closed. src/routes/basket.ts has the same shape. tsc stops reporting after a few syntax errors, so there are likely more behind those two.

This is the second PR today to merge "cleanly" into something that will not compile. The habit that catches it: after any merge or rebase, run npx tsc --noEmit before anything else. A clean merge is not evidence of a working tree.

What I would merge

Rebase on main — not merge — so you resolve against the current shape of each file rather than against a three-way result that kept both. Then:

1. Error codes only; pagination in its own PR.
2. Leave isAdminAuthorized/checkAdminAuth where it is and import it. If the goal was to decouple usage.ts from auth.ts, the honest version is to pass the check in, not to copy it.
3. npx tsc --noEmit and npm test green locally.
4. openapi.yaml is the source of truth and openapi.json is generated — run npm run openapi:gen and npx --yes openapi-typescript@7.13.0 openapi.yaml -o clients/typescript/src/schema.d.ts rather than editing the JSON. Both are CI-guarded byte comparisons.
5. Worth a look at src/errors.ts as it stands on main now: it already owns INTERNAL_ERROR_CODE, correlation IDs and createInternalErrorBody, and docs/http-status-codes.md plus tests/httpStatusCodes.test.ts document the status contract. Your enumerated set should extend that rather than sit beside it — and a test asserting every code in the enum appears in the docs table would keep the two honest.

The core idea is worth having and I would like it in. Ping me when it is split and rebased.

Scanned clean. The idea is right and I want this: one enumerated error-code set, a shared apiError helper, code on every { error } body, the codes documented in OpenAPI, and a guard test that every emitted code stays in the set. That last part — tests/error-codes.test.ts scraping apiError('…') call sites and failing on an unlisted code — is the thing that keeps a taxonomy honest once it exists. I'd like to merge it. It can't go in as it stands.

1. The branch does not compile. Three parse errors, so tsc never gets as far as type checking:

• src/routes/screener.ts:262 — return { … } is closed with }). Drop the ).
• src/routes/basket.ts:93 — same shape, a }/}) mismatch at the end of the handler.
• src/api/usage.ts:65 — a botched splice: const summary = await getUsageSummary(apiKey.id); return reply.send(summary) appears twice with a stray } between them. One copy should go.

npx tsc --noEmit reports all three in about a second.

2. src/api/admin.ts lost three imports. It now imports only hashApiKey from ./auth, but still calls isAdminAuthorized, checkAdminAuth and invalidateApiKey — ten references in the file. Restore the original import rather than re-implementing any of them locally; isAdminAuthorized in particular should have exactly one definition.

3. Five routes lose their API-key scope. config: { scope: … } is removed in five places and added back in none:

That is an authorization regression, and it is unrelated to error codes. Attaching a code to a response body should not touch the route's options object at all. Please put them back and keep the diff to the response shaping.

4. This now needs a rebase. Two PRs landed on main in the last hour that touch the same files:

• 97c50bf (#427) — API versioning; src/app.ts, src/cors.ts, openapi.yaml, README.md and the generated client all moved. Every documented path is now /v1/…, so the error-code documentation has to go onto the versioned paths.
• 263b14f (#429) — cursor pagination; src/routes/candles.ts, src/routes/pairs.ts, src/routes/screener.ts and /pools in src/api/rest.ts were rewritten, and src/routes/screener.ts was restored to the shared runScreener service.

Rebasing on current main first will also resolve item 3 for you, since those route registration lines are the ones that changed.

Worth knowing: #429 arrived with the same three problems — a }) in screener.ts, dropped imports, and the same scopes removed. If a local step is dropping them, it is worth finding, because each one costs a review round trip. Before pushing, npx tsc --noEmit catches items 1 and 2 outright, and npx vitest run catches the rest.

Happy to re-review as soon as it compiles — the error-code work itself I have no objection to.

https://claude.ai/code/session_01USgemLt4Rnz4SGB1Srf3GB

Scanned clean. The rebase landed properly — this is now 0 commits behind main and merges with no conflicts at all, which was the largest of the four problems. Thank you for doing that; it was the tedious one.

None of the other three are addressed, though, and the file damage is worse than yesterday rather than better. I want to be precise, because "it still does not compile" is not useful on its own.

1. 14 parse errors across four files — tsc cannot read them as TypeScript

src/api/usage.ts(65,5)      error TS1005: ',' expected
src/routes/basket.ts(93,3)  error TS1128: Declaration or statement expected
src/routes/screener.ts(262) error TS1005: ';' expected / 'try' expected
src/routes/volumes.ts(125)  error TS1005: 'try' expected

Each is a different symptom of the same thing — a block boundary lost during an edit:

• src/routes/basket.ts is the serious one. It has no top-level declaration left at all. On main the file has export async function registerBasketRoutes(app: FastifyInstance) at line 68; on this branch that line and the app.get(...) beneath it are gone, leaving an orphaned handler body that ends in }) and } with nothing opening them. The route is not registered any more.
• src/routes/volumes.ts has } catch (err) { at line 125 and no try { anywhere in the file.
• src/api/usage.ts has a duplicated tail: const summary = await getUsageSummary(apiKey.id); return reply.send(summary) appears twice around line 61-66, the second copy at an indent level outside the handler, with a stray }.
• src/routes/screener.ts still closes return { … } with }) at line 262, which is the same one I flagged yesterday.

A parse error hides every later error in its file, so expect more to appear once these four are fixed. That is normal — fix them, run tsc again, and work down the new list.

2. Five routes still lose their API-key scope

$ git diff origin/main L437 | grep -cE "^\-.*scope: 'read:"   → 5
$ git diff origin/main L437 | grep -cE "^\+.*scope: 'read:"   → 0

/prices/history, /pools, /candles/:assetA/:assetB, /pairs and /volumes/:asset. Attaching a code to an error body should not touch a route's options object at all, so this is collateral from whatever is eating these files rather than an intended change — which is exactly why it is worth finding the cause rather than re-adding them by hand.

3. src/api/admin.ts still imports only hashApiKey

It calls isAdminAuthorized, checkAdminAuth and invalidateApiKey as well — ten references in the file.

One new thing

src/routes/screener.ts now has console.error('[screener]', error). There is a logging-hygiene test that forbids console.* in non-test source; use createLogger('route:screener') and pass the error as err so pino serialises the stack.

A suggestion, offered genuinely

Four files losing a block boundary in one PR, twice in a row, points at the process rather than at carelessness. If an editor or agent is rewriting whole files, it is worth committing in smaller steps and running npx tsc --noEmit between them — it takes seconds and would have caught every one of these before the push. The error-code idea itself is good and I still want it; what is blocking it is entirely mechanical.

main is green right now (tsc clean, 1,755 tests), so anything tsc or vitest reports on your branch is yours.

https://claude.ai/code/session_01USgemLt4Rnz4SGB1Srf3GB